Skip to content

Conversation

@FriedLongJohns
Copy link
Contributor

@brettle pls review
also @CoolSpy3 thanks!!

sobbing
sim is still broken
tests are still broken
also I probably broke some functionality somewhere
@FriedLongJohns FriedLongJohns requested a review from a team as a code owner January 15, 2025 01:16
sofiebudman and others added 8 commits February 7, 2025 17:30
1. duplicatestrategy to fail
2. no more CachedSparkMax
2. motorcontrollerfactory now configures sparks!!
3. motorErrors checks for null faults
4. swap from smartMotion to MAXmotion in mockedsparkmaxpidcontroller.java
5. cleaner builder pid configs in sparkvelocitypidcontroller.java
6. change from deprecated calculate(double, double) to calculateWithVelocities() in swervemodule and don't persist configs
Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright! Just a few more notes on how motors are handled. After this, I think I'll do another dive through the whole PR, look for any final bugs, apply some formatting fixes, and (barring any other major snags) this should be good to go! Home stretch now!

That being said, simulation is still probably in need of a pretty deep rewrite. I don't think that needs to be part of this PR though, I'd rather get polished 2025 code merged to master, and then sim can be tackled separately.

@timtogan
Copy link
Member

timtogan commented Jan 1, 2026

@CoolSpy3 Alr I am pretty sure I addresed pretty much all your comments if you could if you check everything that would be awesome :D

Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I think all of the major stuff has been addressed. Just two more minor comments. I'll start re-reading everything for anything else that should be fixed before this merges.

Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, here's the last batch of changes. After this, I might just run through and fix some formatting things, and then this should be good to merge!

driveConfig.encoder
.positionConversionFactor(drivePositionFactor)
.velocityConversionFactor(driveVelocityFactor)
.quadratureAverageDepth(2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we setting this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this was added before I started working on this PR, so I am also not entirely sure why we are setting. I'll try to look into if there is a reason to set it.

@timtogan
Copy link
Member

Ok, I basically finished on this batch! Just need to get back to you on this: #107 (comment) I'll prob do it today or tommorow morning. (Also fyi the javadoc doesn't build rn only because ctre javadocs are down)

Copy link
Member

@CoolSpy3 CoolSpy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Here are a couple of additional notes on the past two commits. Also, this comment still appears unresolved.

Comment on lines 129 to 130
links 'https://api.ctr-electronics.com/phoenix/release/java/'
links 'https://api.ctr-electronics.com/phoenix6/release/java/'
Copy link
Member

@CoolSpy3 CoolSpy3 Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some digging. It looks like the pheonix docs have moved. [1] You might want to update any other reference docs we have linking to this.

EDIT: Looks like GitHub doesn't like suggestions in unchanged lines. Basically, release should change to stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants